fix(studio): apply split-bounds epsilon in razor split-all#1404
Conversation
The single-clip razor path guards splits with isSplitTimeWithinBounds, which keeps a SPLIT_BOUNDARY_EPSILON_S margin from each clip edge so a cut never produces a degenerate near-zero slice. The split-all path filtered with raw `splitTime > start && splitTime < end` instead, so it accepted cuts inside that margin (and on clips shorter than two epsilons that the single path always rejects), producing the very degenerate slice the epsilon exists to prevent. Extract the shared predicate canSplitElementAt and a selectSplittableElements helper, and route both razor paths through them so the two stay consistent. Adds unit coverage for the new helpers, including the regression where a sub-epsilon clip with an interior split time must not be selected.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: Clean — ready for stamp. Verified at 8cc7292f; re-verify if HEAD moves before stamp.
A most agreeable little puzzle, and the solution observes the proper form. Two razor paths were disputing the meaning of a clip boundary; the PR settles the quarrel by making them speak through a single magistrate. There are no smoke and mirrors here — only a clean extraction, an honest test, and the deletion of the very drift that #1340 had already taught us to fear.
What the change actually does
packages/studio/src/utils/timelineElementSplit.ts:33-50— introducescanSplitElementAt(el, splitTime)(composition ofcanSplitElementandisSplitTimeWithinBounds) and the list helperselectSplittableElements(elements, splitTime). The JSDoc explicitly names the prior drift and the boundary-epsilon contract, which is the right thing to leave behind for the next archaeologist.packages/studio/src/hooks/useRazorSplit.ts:172— the single-clip path collapses two guards (canSplitElement+isSplitTimeWithinBounds) into one composed predicate. Behaviour unchanged at this site; intent now crisp.packages/studio/src/hooks/useRazorSplit.ts:229— the split-all path, previouslysplitTime > el.start && splitTime < el.start + el.duration, is replaced byselectSplittableElements. This is the substantive fix: split-all now refuses cuts inside theSPLIT_BOUNDARY_EPSILON_S(0.03s) margin AND clips shorter than2 * SPLIT_BOUNDARY_EPSILON_S, in exact agreement with the single-clip path.
Band-aid bar — verdict
This PR is the anti-band-aid shape Miguel keeps asking for. Pattern #1 of the bar (duplicate validation at boundaries with no shared validator) is the exact defect being repaired, not patched over: the two paths now route through one predicate, so they cannot drift again. JSDoc encodes the rule; tests pin the regression. No telemetry/UX asymmetry, no defensive-guard weakening, no contradictory rules, no silent scope gap.
Dispatch-chain audit (the part that catches itself out)
canSplitElement and isSplitTimeWithinBounds remain exported. I confirmed every downstream consumer still routes correctly:
packages/studio/src/player/components/ClipContextMenu.tsx:34—canSplitElement(element) && ["video","audio","img"].includes(element.tag). UI predicate, nosplitTimein scope. Correctly continues to call the cheaper helper.packages/studio/src/components/TimelineToolbar.tsx:160—if (!el || !canSplitElement(el)) return null;. Same shape.packages/studio/src/hooks/useAppHotkeys.ts:332—canSplitElement(element) && …. Same shape.
These are "can this element be split, ever?" sites, not "can I split it at time T?" sites; they have no splitTime and should not migrate to canSplitElementAt. The two-helper split is semantically right, not a leftover.
isSplitTimeWithinBounds continues to be invoked by canSplitElementAt and is still asserted directly in the existing test block at timelineElementSplit.test.ts:13-50. No orphans, no unreachable dispatch.
Test coverage
packages/studio/src/utils/timelineElementSplit.test.ts:69-110 adds three describe blocks that pin precisely the contract:
- The regression — a clip of duration
SPLIT_BOUNDARY_EPSILON_S + 0.01(i.e. shorter than2ε) with a split time strictly inside the clip (interiorTime > start && interiorTime < end). Old raw-inequality filter would have selected it;selectSplittableElements([tiny], interiorTime)correctly returns[]. The twoexpectcalls bracketing the assertion document why the regression exists and protect future readers from "but the time is inside the clip, why is this empty?" confusion. - Lower-clamp boundary acceptance —
selectSplittableElements([el], 2 + SPLIT_BOUNDARY_EPSILON_S)returns[el]. This preserves the inclusivity that #1340 baked in for canvas-clamped edge clicks. - Mixed list with locked/composition/implicit rejection — exercises the
canSplitElementhalf of the composition.
CI and surface
35 checks, all success or appropriately skipped; required checks (Build, Typecheck, Lint, Test, Studio: load smoke, Test: runtime contract, CLI smoke, SDK contract, Fallow audit) all green at 8cc7292f. Diff is +83/-11 across three files, no registry/ churn, no GSAP regime touched, no cross-package SDK shape — well within a single-PR fix's responsibility.
Nits — none worth blocking on
- The JSDoc on
canSplitElementAtalready names the prior bug shape. That is precisely the kind of comment the next reviewer thanks you for. selectSplittableElementsis a one-linefilterovercanSplitElementAt. Some shops would inline; here, naming the operation makes the call site atuseRazorSplit.ts:229self-documenting and matches the existing helper-vocabulary of the file. Fine as is.
Recommendation
Stamp. Same boundary-consistency theme as #1340, properly closed at the source rather than papered over. Re-verify HEAD before clicking approve if any new commits land between this read and the stamp.
— clear at 8cc7292f.
Review by Via
2 participants
Problem
The two razor paths disagree on what counts as a valid split time.
The single-clip path (
handleRazorSplit) guards withisSplitTimeWithinBounds, which keeps aSPLIT_BOUNDARY_EPSILON_Smargin from each clip edge. That margin exists so a cut never lands flush against a boundary and produces a degenerate near-zero slice. It is also the value the timeline canvas clamps edge clicks to, so the inclusive check accepts a clamped click (the asymmetry fixed in #1340).The split-all path (
handleRazorSplitAll) instead filtered with raw inequalities:So split-all accepted cuts inside the epsilon margin, and accepted clips shorter than two epsilons that the single path always rejects, producing the very degenerate slice the epsilon is meant to prevent.
Fix
Extract the shared per-element predicate
canSplitElementAt(which combinescanSplitElementwithisSplitTimeWithinBounds) plus aselectSplittableElementslist helper, and route both razor paths through them. The two paths now apply one rule, so they cannot drift again.Tests
Adds unit coverage for the new helpers, including the regression: a clip shorter than two epsilons, with a split time strictly inside it, must not be selected (the old raw-inequality filter selected it). Full studio suite stays green.
Follow-up to #1340, same boundary-consistency theme.